Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(i): Lint casts & move panics under must funcs #3134

Conversation

shahzadlone
Copy link
Member

Relevant issue(s)

Resolves #3103

Description

  • Fix the BadRequest in the acp handlers to now be panics
  • Make the panics documented under must functions
  • Add the linter to avoid increasing of undocumented panics
  • Ignore linter in existing files outside cli and http (this should either be resolved gradually over time or in a single PR, regardless would prefer to not block this PR because of them)

@shahzadlone shahzadlone added the code quality Related to improving code quality label Oct 15, 2024
@shahzadlone shahzadlone added this to the DefraDB v0.14 milestone Oct 15, 2024
@shahzadlone shahzadlone requested a review from a team October 15, 2024 14:22
@shahzadlone shahzadlone self-assigned this Oct 15, 2024
@shahzadlone shahzadlone changed the title refac(i): Panic in handler over error, & lint panic casts refac(i): Panic in handler over error & lint panic casts Oct 15, 2024
@shahzadlone shahzadlone force-pushed the lone/fix/bad-request-to-internal-server-error branch from 1bcee1c to 1b8b14e Compare October 15, 2024 14:22
@shahzadlone shahzadlone changed the title refac(i): Panic in handler over error & lint panic casts refac(i): Panic in handler & lint panic casts Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 86.58537% with 11 lines in your changes missing coverage. Please review.

Project coverage is 78.05%. Comparing base (bb7843c) to head (d63403f).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
http/handler_lens.go 0.00% 4 Missing ⚠️
http/handler_collection.go 80.00% 2 Missing ⚠️
http/handler_store.go 85.71% 2 Missing ⚠️
http/handler_tx.go 87.50% 2 Missing ⚠️
http/handler.go 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3134      +/-   ##
===========================================
+ Coverage    77.97%   78.05%   +0.07%     
===========================================
  Files          354      354              
  Lines        34660    34669       +9     
===========================================
+ Hits         27025    27058      +33     
+ Misses        6006     5988      -18     
+ Partials      1629     1623       -6     
Flag Coverage Δ
all-tests 78.05% <86.59%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cli/p2p_info.go 100.00% <100.00%> (ø)
cli/purge.go 93.33% <100.00%> (ø)
cli/utils.go 70.27% <100.00%> (+0.55%) ⬆️
http/handler_acp.go 91.18% <100.00%> (+7.73%) ⬆️
http/handler_ccip.go 95.12% <100.00%> (ø)
http/handler_extras.go 100.00% <100.00%> (ø)
http/handler_p2p.go 73.21% <100.00%> (ø)
http/middleware.go 70.00% <100.00%> (ø)
http/utils.go 88.89% <100.00%> (+10.32%) ⬆️
http/handler.go 76.56% <50.00%> (ø)
... and 4 more

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb7843c...d63403f. Read the comment docs.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: This appears to be entirely untested, given the lack of any changes made to test files, and no note of any manual testing in the PR description. I prefer that you add tests, but if you really think manual testing is okay in the short term, please note why.

I also have a question RE the linter excluded file list.

@shahzadlone
Copy link
Member Author

todo: This appears to be entirely untested, given the lack of any changes made to test files, and no note of any manual testing in the PR description. I prefer that you add tests, but if you really think manual testing is okay in the short term, please note why.

Happy to add tests if you have any specific code blocks that I should test, here are all the changes that have been done:

  • Implicit casts that would panic .() moved to mustXYZ functions -> Won't change or need tests as it's the same behavior.
  • HTTP handler functions changed to follow the same practice as all other existing handlers to panic instead of returning 400.
  • Added extra linter (testing not applicable)
  • Some instances of double casts just split into multiple must/``try` checks.

@AndrewSisley
Copy link
Contributor

HTTP handler functions changed to follow the same practice as all other existing handlers to panic instead of returning 400.

It was this I was mostly after - might not be worth the hassle of writing tests for though as I guess it is mostly just the http status code that changed, and that's not something the integration test framework can handle atm I think. Have you manually tested this?

http/middleware.go Outdated Show resolved Hide resolved
http/handler_tx.go Outdated Show resolved Hide resolved
@shahzadlone
Copy link
Member Author

HTTP handler functions changed to follow the same practice as all other existing handlers to panic instead of returning 400.

It was this I was mostly after - might not be worth the hassle of writing tests for though as I guess it is mostly just the http status code that changed, and that's not something the integration test framework can handle atm I think. Have you manually tested this?

Manually some yes, but not all. I am a bit unsure how to test certain situations.

@shahzadlone shahzadlone force-pushed the lone/fix/bad-request-to-internal-server-error branch from 19c5438 to 77bdcdb Compare October 17, 2024 16:45
@shahzadlone shahzadlone changed the title refac(i): Panic in handler & lint panic casts refac(i): Lint panic casts & mustify Oct 17, 2024
@shahzadlone shahzadlone changed the title refac(i): Lint panic casts & mustify refac(i): Lint panic casts & mustify panics Oct 17, 2024
@shahzadlone shahzadlone changed the title refac(i): Lint panic casts & mustify panics refac(i): Lint check casts & mustify panic casts Oct 17, 2024
@shahzadlone shahzadlone force-pushed the lone/fix/bad-request-to-internal-server-error branch from 77bdcdb to 43ade30 Compare October 17, 2024 16:57
@shahzadlone shahzadlone changed the title refac(i): Lint check casts & mustify panic casts refac(i): Lint and mustify panic casts Oct 17, 2024
@shahzadlone shahzadlone changed the title refac(i): Lint and mustify panic casts refac(i): Lint and move panic casts under must Oct 17, 2024
@shahzadlone shahzadlone changed the title refac(i): Lint and move panic casts under must refac(i): Lint & move panic casts under must Oct 17, 2024
@shahzadlone shahzadlone changed the title refac(i): Lint & move panic casts under must refac(i): Move panic casts under must funcs Oct 17, 2024
@shahzadlone shahzadlone changed the title refac(i): Move panic casts under must funcs refactor(i): Move panic casts under must funcs Oct 17, 2024
@shahzadlone shahzadlone changed the title refactor(i): Move panic casts under must funcs refactor(i): Move & lint panic casts under must funcs Oct 19, 2024
@shahzadlone shahzadlone changed the title refactor(i): Move & lint panic casts under must funcs refactor(i): Lint & Move panic casts under must funcs Oct 19, 2024
@shahzadlone shahzadlone changed the title refactor(i): Lint & Move panic casts under must funcs refactor(i): Lint & move panic casts under must funcs Oct 19, 2024
@shahzadlone shahzadlone requested a review from a team October 20, 2024 13:11
@shahzadlone shahzadlone force-pushed the lone/fix/bad-request-to-internal-server-error branch from 43ade30 to ec7e43d Compare October 20, 2024 13:25
@shahzadlone shahzadlone changed the title refactor(i): Lint & move panic casts under must funcs refactor(i): Lint casts & move panics under must funcs Oct 20, 2024
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please make sure Keenan in particular is happy before merge :)

Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming you resolve Fred's remaining todo.

@shahzadlone shahzadlone force-pushed the lone/fix/bad-request-to-internal-server-error branch from ec7e43d to d63403f Compare October 21, 2024 16:15
@shahzadlone shahzadlone merged commit 7799b31 into sourcenetwork:develop Oct 21, 2024
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Related to improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix StatusBadRequest -> StatusInternalServerError for internal errors in http handler
4 participants